Custom Metrics Fixes And Improvements by dustman9000 · Pull Request #149 · openshift/certman-operator

dustman9000 · 2020-07-28T01:50:35Z

Fix SQL statement to conform to crt.sh SQL statements (eg. https://crt.sh/?Identity=devshift.org&exclude=expired&deduplicate=Y&showSQL=Y)
Fix issue where operator-sdk controller-runtime metrics http server port 8080 conflicts with operator-custom-metrics http metrics server port. Verified custom metrics are missing with latest from master on hive-stage.
Remove unused code
Add improved error logs
Defensive check against null private account key to prevent runtime crash

Tested locally with a prometheus instance. Verified proper cert issued counts show up in custom metrics endpoint.

$ kubectl run -it --rm debug --image=radial/busyboxplus:curl --restart=Never -- curl http://172.17.0.3:8080/metrics
# HELP certman_operator_certificate_issue_duration Runtime of issue certificate function in seconds
# TYPE certman_operator_certificate_issue_duration summary
certman_operator_certificate_issue_duration_sum{name="certman-operator"} 0
certman_operator_certificate_issue_duration_count{name="certman-operator"} 0
# HELP certman_operator_certs_in_last_day_devshift_org Report how many certs have been issued for Devshift.org in the last 24 hours
# TYPE certman_operator_certs_in_last_day_devshift_org gauge
certman_operator_certs_in_last_day_devshift_org{name="certman-operator"} 223
# HELP certman_operator_certs_in_last_day_openshift_apps_com Report how many certs have been issued for Openshiftapps.com in the last 24 hours
# TYPE certman_operator_certs_in_last_day_openshift_apps_com gauge
certman_operator_certs_in_last_day_openshift_apps_com{name="certman-operator"} 56
# HELP certman_operator_certs_in_last_week_devshift_org Report how many certs have been issued for Devshift.org in the last 7 days
# TYPE certman_operator_certs_in_last_week_devshift_org gauge
certman_operator_certs_in_last_week_devshift_org{name="certman-operator"} 1413
# HELP certman_operator_certs_in_last_week_openshift_apps_com Report how many certs have been issued for Openshiftapps.com in the last 7 days
# TYPE certman_operator_certs_in_last_week_openshift_apps_com gauge
certman_operator_certs_in_last_week_openshift_apps_com{name="certman-operator"} 371
...

codecov · 2020-07-28T01:54:39Z

Codecov Report

Merging #149 into master will increase coverage by 0.91%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   25.57%   26.49%   +0.91%     
==========================================
  Files          28       28              
  Lines        1474     1423      -51     
==========================================
  Hits          377      377              
+ Misses       1050      998      -52     
- Partials       47       48       +1

Impacted Files	Coverage Δ
cmd/manager/main.go	`0.00% <0.00%> (ø)`
...controller/certificaterequest/issue_certificate.go	`14.01% <0.00%> (-0.41%)`	⬇️
pkg/leclient/lets_encrypt.go	`39.83% <0.00%> (-0.69%)`	⬇️
pkg/localmetrics/crt_sh.go	`0.00% <0.00%> (ø)`
pkg/localmetrics/localmetrics.go	`11.76% <0.00%> (+1.23%)`	⬆️

dustman9000 · 2020-07-28T02:20:18Z

pkg/localmetrics/crt_sh.go


 func getPsqlInfo() string {
-	return fmt.Sprintf("host=%s port=%d user=%s dbname=%s sslmode=disable", CRT_SH_PG_DB_HOSTNAME, CRT_SH_PG_DB_PORT, CRT_SH_PG_DB_USERNAME, CRT_SH_PG_DB_NAME)
+	return fmt.Sprintf("host=%s port=%d user=%s dbname=%s sslmode=disable binary_parameters=yes", CRT_SH_PG_DB_HOSTNAME, CRT_SH_PG_DB_PORT, CRT_SH_PG_DB_USERNAME, CRT_SH_PG_DB_NAME)


Fix for PSQL error: unnamed prepared statement does not exist

* Fix SQL statement to conform to crt.sh SQL statements * Fix issue where operator-sdk controller-runtime metrics http server port 8080 conflicts with operator-custom-metrics http metrics server port * Remove unused clode * Add improved error logs * Defensive check against null private account key to prevent runtime crash

cmd/manager/main.go

fahlmant

As far as operator custom metrics fix goes, lgtm

dustman9000 · 2020-07-28T14:20:02Z

/assign @yithian

cmd/manager/main.go

pkg/controller/certificaterequest/issue_certificate.go

yithian · 2020-07-28T17:51:51Z

What's with the change of metrics from openshift.com to devshift.org?

Co-authored-by: Alex Chvatal <yith@yuggoth.space>

dustman9000 · 2020-07-28T18:03:11Z

What's with the change of metrics from openshift.com to devshift.org?

We do not request LE certificates for openshift.com. I've removed this metric and replaced with the devshift.org instead.

yithian · 2020-07-28T18:19:38Z

/lgtm

@cblecker i wouldn't mind a second set of eyes on the queries.go stuff

cblecker · 2020-07-28T18:20:16Z

/hold

cblecker · 2020-07-31T21:12:05Z

/lgtm
/hold cancel

openshift-ci-robot · 2020-07-31T21:12:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dustman9000, yithian

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cblecker,yithian]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dustman9000 requested review from c-e-brumm, cblecker, dofinn, fahlmant and jharrington22 July 28, 2020 01:50

dustman9000 force-pushed the prom-custom-metrics branch from 1204ccc to fe83821 Compare July 28, 2020 02:03

dustman9000 commented Jul 28, 2020

View reviewed changes

dustman9000 force-pushed the prom-custom-metrics branch from fe83821 to bf94569 Compare July 28, 2020 02:33

dustman9000 force-pushed the prom-custom-metrics branch from bf94569 to 95eb5c4 Compare July 28, 2020 02:35

fahlmant reviewed Jul 28, 2020

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

fahlmant reviewed Jul 28, 2020

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

fahlmant reviewed Jul 28, 2020

View reviewed changes

dustman9000 requested a review from yithian July 28, 2020 14:19

dustman9000 assigned yithian Jul 28, 2020

dustman9000 commented Jul 28, 2020

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

Disable controller-runtime metrics server

e526999

2uasimojo reviewed Jul 28, 2020

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

Minor code optimization.

b29d829

yithian reviewed Jul 28, 2020

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

yithian suggested changes Jul 28, 2020

View reviewed changes

pkg/controller/certificaterequest/issue_certificate.go Outdated Show resolved Hide resolved

pkg/controller/certificaterequest/issue_certificate.go Outdated Show resolved Hide resolved

dustman9000 and others added 2 commits July 28, 2020 11:01

Update pkg/controller/certificaterequest/issue_certificate.go

0f98616

Co-authored-by: Alex Chvatal <yith@yuggoth.space>

Update pkg/controller/certificaterequest/issue_certificate.go

d6d7b1a

Co-authored-by: Alex Chvatal <yith@yuggoth.space>

dustman9000 closed this Jul 28, 2020

dustman9000 reopened this Jul 28, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2020

openshift-ci-robot assigned cblecker Jul 31, 2020

openshift-merge-robot merged commit 576cb70 into openshift:master Jul 31, 2020

Comments

Conversation

dustman9000 commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dustman9000 Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fahlmant left a comment

Choose a reason for hiding this comment

Uh oh!

dustman9000 commented Jul 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yithian commented Jul 28, 2020

Uh oh!

dustman9000 commented Jul 28, 2020

Uh oh!

yithian commented Jul 28, 2020

Uh oh!

cblecker commented Jul 28, 2020

Uh oh!

cblecker commented Jul 31, 2020

Uh oh!

openshift-ci-robot commented Jul 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dustman9000 commented Jul 28, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading